Skip to content

Fix legacy code, typos, suppress run_time warnings, added further support for PyVista visualisations, flagged all kernel killing code raised in Issue #46#47

Merged
ESadek-MO merged 16 commits into
scitools-classroom:legacy-fixfrom
harley-kelly:main
Jul 31, 2025
Merged

Fix legacy code, typos, suppress run_time warnings, added further support for PyVista visualisations, flagged all kernel killing code raised in Issue #46#47
ESadek-MO merged 16 commits into
scitools-classroom:legacy-fixfrom
harley-kelly:main

Conversation

@harley-kelly

Copy link
Copy Markdown

Fix legacy code for loading data with iris. Corrected any and all typos. Added lines to suppress run_time warnings which bulk up output. Added further support for PyVista visualisations using trame. Flagged all kernel killing code raised in Issue #46 in bold red font.

code in the following notebooks:
- notebooks/Exercise_01.ipynb
- notebooks/Sec_01_Load_and_Examine.ipynb
- notebooks/Sec_03_Plotting.ipynb
- notebooks/Sec_04_Regridding.ipynb
- notebooks/Sec_05_RegionExtraction.ipynb
- notebooks/testdata_fetching.py
…window when using VSCode, flagged where kernel crashes take place.
@CLAassistant

CLAassistant commented Apr 17, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@bjlittle bjlittle requested a review from ESadek-MO April 23, 2025 09:39
@ESadek-MO ESadek-MO self-assigned this Apr 24, 2025

@ESadek-MO ESadek-MO left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @harley-kelly! It's great for this repo to get some TLC.

The majority of these comments are to clean up any fingerprints. Once those are removed, I should probably have another look, as the outputs can make it hard to see the trees for the wood!

If there's any comments you disagree with, please do so, there's a chance I've lost some context due to the outputs etc.

Comment thread notebooks/Exercise_01.ipynb Outdated
Comment thread notebooks/Exercise_01.ipynb Outdated
Comment thread notebooks/Exercise_01.ipynb Outdated
Comment thread notebooks/Exercise_01.ipynb Outdated
"execution_count": 8,
"id": "b8eb1ce2-2160-4c3f-b97b-4b09946d84cc",
"metadata": {
"tags": []

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github won't allow me to comment on relevant lines, but below, steps 3 and 4 are labelled the wrong way round, seems within scope of this PR.

Comment thread notebooks/Sec_04_Regridding.ipynb Outdated
Comment thread notebooks/Sec_04_Regridding.ipynb Outdated
Comment thread notebooks/Sec_04_Regridding.ipynb Outdated
Comment thread notebooks/Sec_04_Regridding.ipynb Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub won't allow me to see this in the browser; I'll review this file later on more manually.

In the meantime, please could you double check that there's no outputs/execution numbers included?

harley-kelly and others added 13 commits April 25, 2025 13:57
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>
Co-authored-by: Elias <110238618+ESadek-MO@users.noreply.github.com>

@ESadek-MO ESadek-MO left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @harley-kelly,

Sorry for the delay on getting back to this, we've been a bit busy with sprinting.

There're still a few fingerprints here, but for the most part it seems good.

We are really grateful that you've made this contribution, and I'm concious you might not have time to keep referring back! If you do, great, that would be ideal, but if you don't and you'd like us to take over the pull request (with credit to you, of course), we'd be happy to!

Comment on lines +1703 to +1704
"iris.FUTURE.date_microseconds = True\n",
"\n",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still needed!

"um_cube = iris.load_cube(um_filepth,'air_temperature')\n",
"#print(um_cube)\n",
"print(um_cube.mesh)\n"
"print('You can see the data here: \\n \\n', um_cube)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"print('You can see the data here: \\n \\n', um_cube)"
"print('You can see the data here: \n \n', um_cube)"

"metadata": {
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"display_name": "everyday",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"display_name": "everyday",
"display_name": "Python 3 (ipykernel)",

{
"cell_type": "code",
"execution_count": null,
"execution_count": 68,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"execution_count": 68,
"execution_count": null,

{
"cell_type": "code",
"execution_count": null,
"execution_count": 74,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"execution_count": 74,
"execution_count": null,

Comment on lines +503 to +516
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"[[792.0181318972558]\n",
" [26.95742764760166]\n",
" [146.36362300827432]\n",
" [166.02688348059408]\n",
" [427.4766774994878]\n",
" [263.7582442600861]]\n"
]
}
],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"[[792.0181318972558]\n",
" [26.95742764760166]\n",
" [146.36362300827432]\n",
" [166.02688348059408]\n",
" [427.4766774994878]\n",
" [263.7582442600861]]\n"
]
}
],
"outputs": [],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file needs the Jupyter fingerprint removed as well!

@ESadek-MO ESadek-MO changed the base branch from main to legacy-fix July 31, 2025 15:30
@ESadek-MO

Copy link
Copy Markdown

Comments haven't been addressed for about two months. I'll merge this into a separate branch, and then continue the work myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants